Change `OpLogFilter::agent_id` to be optional #826

henry0715-dev · 2024-09-11T06:24:56Z

Close #724

The RocksDB keys in the oplog are currently in the form agent_id-0-timestamp. This change modifies the keys to the form timestamp-sequence.

codecov · 2024-09-11T06:26:17Z

Codecov Report

Attention: Patch coverage is 89.36937% with 59 lines in your changes missing coverage. Please review.

Project coverage is 77.55%. Comparing base (2d2c6b8) to head (19d4409).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
src/ingest.rs	15.15%	28 Missing ⚠️
src/graphql.rs	92.41%	11 Missing ⚠️
src/storage/migration.rs	92.70%	7 Missing ⚠️
src/ingest/generation.rs	77.77%	6 Missing ⚠️
src/storage.rs	89.36%	5 Missing ⚠️
src/graphql/log.rs	91.30%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #826      +/-   ##
==========================================
+ Coverage   77.01%   77.55%   +0.53%     
==========================================
  Files          31       32       +1     
  Lines       25569    26061     +492     
==========================================
+ Hits        19692    20211     +519     
+ Misses       5877     5850      -27

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

src/storage/migration.rs

src/graphql/client/schema/op_log_raw_events.graphql

src/ingest/generation.rs

sophie-cluml · 2024-11-13T11:07:30Z

src/ingest/generation.rs

+            let mut last_reset_day = self
+                .last_reset_date
+                .lock()
+                .expect("last_reset_date should be exist.");


I have two questions regarding this line:

Could you run grammar check for the expect message please?

Could you help me understand the choice of expect here please ? To me it seems like we need to use something like panic! according to our guideline.

I will change the source code from Mutex to RwLock as below.
Therefore, there seems to be no need to discuss expect and panic!.

pub struct SequenceGenerator { counter: AtomicUsize, last_reset_date: RwLock<u32>, } pub(crate) async fn generate_sequence_number(&self) -> usize { let current_date_time = SequenceGenerator::get_current_date_time(); { let last_reset_day = self.last_reset_date.read().await; if *last_reset_day == current_date_time { return self.counter.fetch_add(1, Ordering::Relaxed); } } { let mut last_reset_day = self.last_reset_date.write().await; self.counter.store(1, Ordering::Release); *last_reset_day = current_date_time; } self.counter.fetch_add(1, Ordering::Relaxed) }

src/storage/migration/migration_structures.rs

src/ingest.rs

CHANGELOG.md

src/graphql/log/tests.rs

CHANGELOG.md

sophie-cluml

I think the version in Cargo.toml file should be updated to have suffix '-alpha.2'.

henry0715-dev · 2024-11-20T04:12:08Z

I think the version in Cargo.toml file should be updated to have suffix '-alpha.2'.

It's done.
Thanks you.

CHANGELOG.md

henry0715-dev · 2024-11-21T08:07:20Z

@sophie-cluml
I would like to ask if this PR is included in release version 0.23.0.

sophie-cluml · 2024-11-21T08:55:34Z

@henry0715-dev, I believe there are still a few areas that need to be addressed in the current PR, and with the demand for the upcoming release, I'm afraid it may not be feasible to include this PR in the 0.23.0 release. I apologize for any inconvenience, and I appreciate your understanding.

henry0715-dev · 2024-11-22T01:20:35Z

@sophie-cluml
You want to work by modifying the version of that PR to 0.24.0-alpha.1.
If you need to set up another version, please let me know.

sophie-cluml · 2024-11-22T03:20:54Z

As a reply to #826 (comment), I would appreciate if you could update the version to 0.24.0-alpha.1 for this PR and make consecutive changes.

henry0715-dev · 2024-11-22T07:22:00Z

As a reply to #826 (comment), I would appreciate if you could update the version to 0.24.0-alpha.1 for this PR and make consecutive changes.

It's done.
Thanks you.

src/storage.rs

CHANGELOG.md

src/ingest/generation.rs

sophie-cluml · 2024-11-27T10:45:54Z

I would like to merge this PR after @henry0715-dev resolves a conflict in the CHANGELOG. As far as I know, David and Kone are interested in this PR, so I'd like to ask @kimhanbeom and @sehkone if there are any remaining concerns, so that I need to put hold on merging this PR.

msk · 2024-11-27T19:41:23Z

I noticed the b'0' separator is still used in the new key format. While it was necessary in the current format to resolve sorting issues, it seems redundant in the new format, where the fixed-size timestamp ensures proper lexicographical order.

To illustrate, in the current format:

Two agent IDs, 0x77 and 0x7766, with timestamps starting at 0x99, would produce keys:
- 0x770099...
- 0x77660099...
  Without the b'0', they become 0x7799... and 0x776699..., causing incorrect sorting (0x776699 comes before 0x7799).

In the new format, this problem doesn’t arise because the fixed-size timestamp ensures keys like:

0x990077...
0x99007766...
which sort correctly even without b'0'.

Removing b'0' can simplify the format and reduce overhead.

henry0715-dev · 2024-11-28T00:34:30Z

Removing b'0' can simplify the format and reduce overhead.

I applied the content to the source code.
Thank you.

sophie-cluml · 2024-11-28T06:55:07Z

src/storage.rs

+
+    pub fn mid_key(mut self, key: usize) -> Self {
+        let mid_key = key.to_be_bytes();
+        self.pre_key.reserve(mid_key.len() + 1);


I don't think we need + 1 here, because we are not padding the key with b'0' anymore, and the same for the other relevant places.

Additionally
I modified the following in the lower_closed_bound_start_key, upper_open_bound_start_key, and upper_closed_bound_start_key functions.

as-is
self.pre_key.reserve(TIMESTAMP_SIZE+1);
to-be
self.pre_key.reserve(TIMESTAMP_SIZE);

Close #724 The RocksDB keys in the `oplog` are currently in the form agent_id-0-timestamp. This change modifies the keys to the form timestamp-sequence.

henry0715-dev force-pushed the henry/op-log-filter branch from 880ab3d to 3c66f2e Compare October 23, 2024 08:34

henry0715-dev force-pushed the henry/op-log-filter branch from 3c66f2e to d173f83 Compare November 4, 2024 08:49

henry0715-dev changed the title ~~[Draft] Temp commit for OpLogFilter development direction~~ Change OpLogFilter::agent_id to be optional Nov 4, 2024

henry0715-dev marked this pull request as ready for review November 4, 2024 08:55

henry0715-dev force-pushed the henry/op-log-filter branch 2 times, most recently from 6b211c0 to be77ab4 Compare November 5, 2024 03:53

henry0715-dev requested a review from kimhanbeom November 5, 2024 06:23

henry0715-dev force-pushed the henry/op-log-filter branch from be77ab4 to fe5bc59 Compare November 7, 2024 03:56

sophie-cluml self-requested a review November 7, 2024 06:39

kimhanbeom reviewed Nov 8, 2024

View reviewed changes

src/storage/migration.rs Outdated Show resolved Hide resolved

henry0715-dev force-pushed the henry/op-log-filter branch from fe5bc59 to 4bef708 Compare November 8, 2024 08:15

sophie-cluml reviewed Nov 13, 2024

View reviewed changes

src/storage/migration.rs Outdated Show resolved Hide resolved

sophie-cluml reviewed Nov 13, 2024

View reviewed changes

src/storage/migration.rs Outdated Show resolved Hide resolved

sophie-cluml reviewed Nov 13, 2024

View reviewed changes

CHANGELOG.md Show resolved Hide resolved

henry0715-dev force-pushed the henry/op-log-filter branch 2 times, most recently from e67329d to 97ce434 Compare November 14, 2024 05:34

henry0715-dev requested a review from sophie-cluml November 14, 2024 05:48

sophie-cluml reviewed Nov 15, 2024

View reviewed changes

src/graphql/log/tests.rs Outdated Show resolved Hide resolved

sehkone self-requested a review November 15, 2024 05:13

henry0715-dev force-pushed the henry/op-log-filter branch 2 times, most recently from ff0dcae to bf40057 Compare November 15, 2024 09:03

sophie-cluml reviewed Nov 19, 2024

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

henry0715-dev force-pushed the henry/op-log-filter branch from bf40057 to 05d610b Compare November 20, 2024 01:13

henry0715-dev requested a review from sophie-cluml November 20, 2024 02:05

sophie-cluml reviewed Nov 20, 2024

View reviewed changes

henry0715-dev force-pushed the henry/op-log-filter branch from 05d610b to d868535 Compare November 20, 2024 04:11

sophie-cluml reviewed Nov 20, 2024

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

CHANGELOG.md Outdated Show resolved Hide resolved

CHANGELOG.md Outdated Show resolved Hide resolved

henry0715-dev force-pushed the henry/op-log-filter branch from d868535 to 931ac8c Compare November 21, 2024 00:43

henry0715-dev requested a review from sophie-cluml November 21, 2024 01:12

This was referenced Nov 21, 2024

Update Giganto protocol version to 0.24.0-alpha.1 aicers/reproduce#542

Open

Update Giganto protocol version to 0.24.0-alpha.1 aicers/crusher#120

Open

henry0715-dev force-pushed the henry/op-log-filter branch from 931ac8c to cbbde80 Compare November 22, 2024 03:53

sophie-cluml reviewed Nov 25, 2024

View reviewed changes

src/storage.rs Outdated Show resolved Hide resolved

CHANGELOG.md Outdated Show resolved Hide resolved

henry0715-dev force-pushed the henry/op-log-filter branch from cbbde80 to 7370c47 Compare November 26, 2024 00:53

sophie-cluml reviewed Nov 26, 2024

View reviewed changes

src/ingest/generation.rs Outdated Show resolved Hide resolved

henry0715-dev force-pushed the henry/op-log-filter branch from 7370c47 to c2a2bf5 Compare November 26, 2024 02:26

sophie-cluml reviewed Nov 26, 2024

View reviewed changes

src/ingest/generation.rs Outdated Show resolved Hide resolved

henry0715-dev force-pushed the henry/op-log-filter branch from c2a2bf5 to 4a1cbf5 Compare November 26, 2024 06:22

henry0715-dev requested a review from sophie-cluml November 26, 2024 06:26

sophie-cluml approved these changes Nov 26, 2024

View reviewed changes

henry0715-dev force-pushed the henry/op-log-filter branch from 4a1cbf5 to cfb4bde Compare November 28, 2024 00:32

henry0715-dev requested a review from sophie-cluml November 28, 2024 00:43

sophie-cluml reviewed Nov 28, 2024

View reviewed changes

Change OpLogFilter::agent_id to be optional

19d4409

Close #724 The RocksDB keys in the `oplog` are currently in the form agent_id-0-timestamp. This change modifies the keys to the form timestamp-sequence.

henry0715-dev force-pushed the henry/op-log-filter branch from cfb4bde to 19d4409 Compare November 28, 2024 08:44

sophie-cluml approved these changes Dec 2, 2024

View reviewed changes

sophie-cluml merged commit 07b5a3c into main Dec 2, 2024
10 checks passed

sophie-cluml deleted the henry/op-log-filter branch December 2, 2024 09:53

sophie-cluml mentioned this pull request Dec 4, 2024

Fix incorrect INGEST_VERSION_REQ and PUBLISH_VERSION_REQ #920

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change `OpLogFilter::agent_id` to be optional #826

Change `OpLogFilter::agent_id` to be optional #826

henry0715-dev commented Sep 11, 2024 •

edited

Loading

codecov bot commented Sep 11, 2024 •

edited

Loading

sophie-cluml Nov 13, 2024

henry0715-dev Nov 14, 2024

sophie-cluml left a comment

henry0715-dev commented Nov 20, 2024

henry0715-dev commented Nov 21, 2024

sophie-cluml commented Nov 21, 2024

henry0715-dev commented Nov 22, 2024

sophie-cluml commented Nov 22, 2024 •

edited

Loading

henry0715-dev commented Nov 22, 2024

sophie-cluml commented Nov 27, 2024

msk commented Nov 27, 2024

henry0715-dev commented Nov 28, 2024

sophie-cluml Nov 28, 2024

henry0715-dev Nov 28, 2024

Change OpLogFilter::agent_id to be optional #826

Change OpLogFilter::agent_id to be optional #826

Conversation

henry0715-dev commented Sep 11, 2024 • edited Loading

codecov bot commented Sep 11, 2024 • edited Loading

Codecov Report

sophie-cluml Nov 13, 2024

Choose a reason for hiding this comment

henry0715-dev Nov 14, 2024

Choose a reason for hiding this comment

sophie-cluml left a comment

Choose a reason for hiding this comment

henry0715-dev commented Nov 20, 2024

henry0715-dev commented Nov 21, 2024

sophie-cluml commented Nov 21, 2024

henry0715-dev commented Nov 22, 2024

sophie-cluml commented Nov 22, 2024 • edited Loading

henry0715-dev commented Nov 22, 2024

sophie-cluml commented Nov 27, 2024

msk commented Nov 27, 2024

henry0715-dev commented Nov 28, 2024

sophie-cluml Nov 28, 2024

Choose a reason for hiding this comment

henry0715-dev Nov 28, 2024

Choose a reason for hiding this comment

Change `OpLogFilter::agent_id` to be optional #826

Change `OpLogFilter::agent_id` to be optional #826

henry0715-dev commented Sep 11, 2024 •

edited

Loading

codecov bot commented Sep 11, 2024 •

edited

Loading

sophie-cluml commented Nov 22, 2024 •

edited

Loading